Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

enhancement: Make results tiltle editable and URL persistent #774

Open
wants to merge 36 commits into
base: main
Choose a base branch
from

Conversation

FatumaA
Copy link

@FatumaA FatumaA commented Oct 9, 2024

This PR makes the Results page title editable and persists in the URL state.

Before, the Results view had "Results" as the title. This title could not be customized and was not part of the URL parameters.

Now:

A screenshot of the results page with an arrow pointing to the title and a thick red line below the URL params at the top of the image

Before the changes:

A screenshot of the results page with an arrow pointing to the title and a thick red line below the URL params at the top of the image

Closes #769

@FatumaA FatumaA marked this pull request as draft October 9, 2024 14:57
Copy link

netlify bot commented Oct 9, 2024

Deploy Preview for mozilla-perfcompare ready!

Name Link
🔨 Latest commit 7733853
🔍 Latest deploy log https://app.netlify.com/sites/mozilla-perfcompare/deploys/67182c8910c2d5000865c03e
😎 Deploy Preview https://deploy-preview-774--mozilla-perfcompare.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@FatumaA FatumaA changed the title enhancement: Make results tiltle editable and URLpersistent [WIP] enhancement: Make results tiltle editable and URLpersistent Oct 9, 2024
@FatumaA FatumaA changed the title [WIP] enhancement: Make results tiltle editable and URLpersistent [WIP] enhancement: Make results tiltle editable and URL persistent Oct 9, 2024
@FatumaA FatumaA marked this pull request as ready for review October 9, 2024 15:20
@FatumaA FatumaA changed the title [WIP] enhancement: Make results tiltle editable and URL persistent enhancement: Make results tiltle editable and URL persistent Oct 9, 2024
@syedbarimanjan
Copy link

syedbarimanjan commented Oct 9, 2024

Make the pr description better like adding pictures and explaining your implementaion.

Also you should look into the CI tests which are failing, read this section of the project readme which will help you with it.

@hibaa03
Copy link

hibaa03 commented Oct 9, 2024

Hi, could you add the screenshots to the description for more clarification? Also run the tests and validations locally to see which ones are failing and work on the issues as you will see on your terminal. Maybe its due to jest snapshot which you will have to update

@FatumaA
Copy link
Author

FatumaA commented Oct 10, 2024

Oops forgot to update the description from when it was on draft mode

Copy link
Contributor

@julienw julienw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this looks pretty good as a first draft!

I left a comment about the mechanism to change the search params.

Otherwise I think that it should be make editable after a click on a button.

So here is how it could look like:

  1. at load time, we still have the "Results" by default (or the value in the URL if present) => the good thing is that most of the tests would still work.
  2. add a little edit icon (with MUI's Edit icon) next to this text. => i'd put it on the right of the text, this would look like : Results [Edit icon] | Compare with a base
  3. when the user clicks the edit icon, then the text changes into an input like you did.

It would be good to extract this functionality into a separate react component.

Finally it would be good to add a test to test this functionality.

Thanks again!

src/components/CompareResults/ResultsMain.tsx Outdated Show resolved Hide resolved
@FatumaA
Copy link
Author

FatumaA commented Oct 11, 2024

Thanks, this looks pretty good as a first draft!

I left a comment about the mechanism to change the search params.

Otherwise I think that it should be make editable after a click on a button.

So here is how it could look like:

  1. at load time, we still have the "Results" by default (or the value in the URL if present) => the good thing is that most of the tests would still work.
  2. add a little edit icon (with MUI's Edit icon) next to this text. => i'd put it on the right of the text, this would look like : Results [Edit icon] | Compare with a base
  3. when the user clicks the edit icon, then the text changes into an input like you did.

It would be good to extract this functionality into a separate react component.

Finally it would be good to add a test to test this functionality.

Thanks again!

Thanks for the review. Working on it 🙂.

@FatumaA FatumaA closed this Oct 14, 2024
@FatumaA FatumaA reopened this Oct 14, 2024
@FatumaA FatumaA marked this pull request as draft October 14, 2024 09:42
@FatumaA FatumaA marked this pull request as ready for review October 14, 2024 14:53
@FatumaA
Copy link
Author

FatumaA commented Oct 14, 2024

Thanks, this looks pretty good as a first draft!

I left a comment about the mechanism to change the search params.

Otherwise I think that it should be make editable after a click on a button.

So here is how it could look like:

  1. at load time, we still have the "Results" by default (or the value in the URL if present) => the good thing is that most of the tests would still work.
  2. add a little edit icon (with MUI's Edit icon) next to this text. => i'd put it on the right of the text, this would look like : Results [Edit icon] | Compare with a base
  3. when the user clicks the edit icon, then the text changes into an input like you did.

It would be good to extract this functionality into a separate react component.

Finally it would be good to add a test to test this functionality.

Thanks again!

Hi,
I think this is ok now. Except for the tests. I've commented out the last 2 tests because they keep timing out and I'm not sure how to go about that.

I'd appreciate a look there, thank you 🙏🏽

@hibaa03
Copy link

hibaa03 commented Oct 14, 2024

Make sure your internet connection isn't slow and you release some memory of your system if needed. And run the tests using npm test -- --testTimeout 60000 if they're still timing out.

@FatumaA
Copy link
Author

FatumaA commented Oct 14, 2024

Make sure your internet connection isn't slow and you release some memory of your system if needed. And run the tests using npm test -- --testTimeout 60000 if they're still timing out.

Could be slow internet, I did increase timeout and same result

Copy link
Contributor

@julienw julienw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!
I left a lot of comments but I'd like to highlight that this is very good work still. This is difficult to get right! Hopefully we can get this patch to a mergeable state :-)

src/components/CompareResults/ResultsTitle.tsx Outdated Show resolved Hide resolved
src/components/CompareResults/ResultsTitle.tsx Outdated Show resolved Hide resolved
src/components/CompareResults/ResultsTitle.tsx Outdated Show resolved Hide resolved
src/components/CompareResults/ResultsTitle.tsx Outdated Show resolved Hide resolved
src/components/CompareResults/ResultsTitle.tsx Outdated Show resolved Hide resolved
src/components/CompareResults/ResultsTitle.tsx Outdated Show resolved Hide resolved
src/components/CompareResults/ResultsTitle.tsx Outdated Show resolved Hide resolved
@FatumaA
Copy link
Author

FatumaA commented Oct 22, 2024

Thanks! I left a lot of comments but I'd like to highlight that this is very good work still. This is difficult to get right! Hopefully we can get this patch to a mergeable state :-)

Thank you Julien 😊. I think you can take another look now.

Copy link

codecov bot commented Oct 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.62%. Comparing base (3a555e3) to head (7733853).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #774      +/-   ##
==========================================
+ Coverage   91.46%   91.62%   +0.15%     
==========================================
  Files          88       89       +1     
  Lines        2332     2376      +44     
  Branches      434      445      +11     
==========================================
+ Hits         2133     2177      +44     
  Misses        176      176              
  Partials       23       23              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Carla-Moz
Copy link
Contributor

@FatumaA Hi I'd like to let you know you've been selected to go on the next round! Congrats!
Please see the matrix channel for details https://chat.mozilla.org/#/room/#perfcompare-outreachy:mozilla.org

@FatumaA
Copy link
Author

FatumaA commented Oct 24, 2024

@FatumaA Hi I'd like to let you know you've been selected to go on the next round! Congrats! Please see the matrix channel for details https://chat.mozilla.org/#/room/#perfcompare-outreachy:mozilla.org

Thank you Carla! 🙏🏽

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Results Page: Allow users to change the page title of PerfCompare and have the title persist in the url
5 participants